fix(net): harden the loopback transport guard across all HTTP clients - #804
Conversation
benw5483
changed the title
0acc438 to
b26c7b8
Compare
Actual Adversarial ReviewKey findingsNo actionable defects survived validation. The shared predicate parses the URL, rejects userinfo, and accepts only clear-text loopback identities. All six affected constructors call it; their non-loopback paths retain The implementation also meets Architecture intentNo architecture-intent decision is needed for this change. Review metadataImportant All tracked findings are addressed at head Compared: upstream |
wattswolf
left a comment
wattswolf
left a comment
There was a problem hiding this comment.
Clean security fix: swaps the bypassable starts_with prefix checks for a real URL parse that matches the actual host, rejects userinfo smuggling, and only relaxes https_only for genuine loopback. All six call sites converted consistently, well covered by unit tests, CI green. Good to merge.
austinborn
left a comment
austinborn
left a comment
There was a problem hiding this comment.
Reviewed the full diff at head b26c7b88. This is the one with a real bug behind it, and the fix is correct.
No blocking findings. Approving.
The bug this closes
The old guard decided "is this loopback?" with str::starts_with on the raw URL, which is a prefix test standing in for a host test. http://localhost.evil.com satisfies starts_with("http://localhost"). So does http://127.0.0.1.evil.com for the IPv4 form. Matching means https_only gets switched off, and a caller-controlled base URL then carries credentials over clear-text HTTP to a host the attacker owns. is_loopback_http_url parses the URL and compares the real host instead, which is the right shape: the userinfo rejection also kills http://127.0.0.1@evil.com, where the connection's actual destination is the part after the @.
Comparing against Ipv4Addr::LOCALHOST and Ipv6Addr::LOCALHOST rather than the wider 127.0.0.0/8 range is stricter than it strictly needs to be, and the doc comment owns that choice, so I read it as deliberate.
Verifying the "across all HTTP clients" claim
The title makes a coverage claim, so I checked it against the tree at head rather than the file list:
- All six production sites that gate
https_onlyon a loopback decision now route throughis_loopback_http_url:api/client.rs:65,auth/oauth.rs:155, bothmodel_cache.rssites (160, 251),runner/anthropic_api.rs:87, and thehttps_only(false)branch atrunner/openai_api.rs:156. - No raw prefix-match loopback guard survives in production code. The three remaining
starts_with("http://…")hits are two test assertions and the doc comment innet.rsdescribing the old pattern. - Every
Client::builder()without anhttps_onlycall sits inside a#[cfg(test)]module, past the single#[cfg(test)]marker in each file. - No
danger_accept_invalid_certsor other TLS escape anywhere in the crate.
The claim holds. The test table covering localhost.evil.com, 127.0.0.1.evil.com, 127.0.0.1@evil.com, and the https/no-host/garbage cases is the right set, and pinning url (already an indirect dependency via reqwest) is the cheap way to get an actual parser.
Merge order with #801
#801 touches build_http_client too, only to widen it to pub(crate), and its hunk overlaps this one. Whichever lands second will conflict in src/auth/oauth.rs. This one should land first, and when #801 is rebased the resolution needs to keep is_loopback_http_url rather than restore the starts_with form. Worth saying out loud, because a conflict resolved by taking the incoming side would silently undo this fix while leaving the PR looking merged.
Posted by the operator's software factory.
• City:factory-main· Agent:local-core.builder-1
• On behalf of: @austinborn
`build_http_client` decided whether to relax the HTTPS-only transport
guard with `str::starts_with("http://localhost")` / `127.0.0.1` / `[::1]`
on the raw URL. That prefix match is satisfied by attacker-controlled
hosts — `http://localhost.evil.com`, `http://127.0.0.1.evil.com`, and the
userinfo form `http://127.0.0.1@evil.com` (the connection's real host is
`evil.com`) — so a caller who controls the base URL could disable
`https_only` and send credentials to a non-loopback host in clear text.
Replace the prefix check with a parsed-URL match (`is_loopback_http_url`):
require the `http` scheme, reject any userinfo component, and match the
host exactly against `localhost` / `127.0.0.1` / `::1`. Genuine loopback
dev endpoints still work; every other URL is forced through HTTPS.
Add negative tests for the three bypass URLs (the guard now refuses them)
and positive tests for real loopback and https endpoints.
Generated by the operator's software factory.
City: factory-main · Agent: local-core.builder-2
On behalf of: @benw5483
Co-Authored-By: Actual Factory Bot <factory-bot@actual-software.invalid>
…uard The bypassable loopback check fixed in the auth client was duplicated, with the same `str::starts_with` shape, in every other HTTP-client constructor in the crate: the platform API client (which attaches the login-session bearer to every authenticated call), the model-cache probes, and the OpenAI / Anthropic runner clients. Each could be tricked by `http://localhost.evil.com` or `http://127.0.0.1@evil.com` into disabling `https_only` for a non-loopback host. Extract the hardened check into a single `crate::net::is_loopback_http_url` (parse with `url::Url`, require the `http` scheme, reject userinfo, and match the host exactly against `localhost` / `127.0.0.1` / `::1`) and route every one of these constructors through it, so there is one guard and no divergent copies. Behavior is preserved for genuine loopback dev endpoints; every other URL is forced through HTTPS. The predicate's unit tests move to the new module alongside it. Generated by the operator's software factory. City: factory-main · Agent: local-core.builder-2 On behalf of: @benw5483 Co-Authored-By: Actual Factory Bot <factory-bot@actual-software.invalid>
b26c7b8 to
5a0d83a
Compare
95dafbf to
5a0d83a
Compare
4 participants
Summary
Several HTTP clients in this crate allow clear-text
httpfor a local dev / mock server by disabling reqwest'shttps_only, and they all decided "is this loopback?" with a raw string prefix check (starts_with("http://localhost")/127.0.0.1/[::1]). That prefix match is satisfied by attacker-controlled hosts:http://localhost.evil.com/http://127.0.0.1.evil.com— lookalike hosts that merely start with the loopback literal;http://127.0.0.1@evil.com— a userinfo prefix whose real connection host isevil.com.Any of these made a client treat a non-loopback host as loopback, disabling
https_onlyand letting credentials be sent in clear text when the caller controls the base URL (--api-url/ACTUAL_API_URL).The same bypassable check was duplicated across ~6 constructors: the auth client (
auth/oauth.rs), the platform API client (api/client.rs, which attaches the login-session bearer to every authenticated call), the model-cache probes (model_cache.rs), and the OpenAI / Anthropic runner clients.This extracts one hardened helper —
net::is_loopback_http_url— and routes every constructor through it. The helper parses the URL, requires thehttpscheme, rejects any userinfo component, and matches the host exactly againstlocalhost/127.0.0.1/::1. Genuine loopback dev endpoints keep working; every other URL is forced through HTTPS. No divergent copies of the check remain.Test plan
httpsendpoints behave correctly; the auth client's integration tests cover its use of the helper.cargo testgreen;cargo fmt --checkandcargo clippy -- -D warningsclean.grepconfirms nostarts_withloopback guard remains — every client routes through the shared helper.localhost/127.0.0.1/::1) matches the intended dev endpoints for all of these clients.